Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write registry contract #3

Merged
merged 3 commits into from
Oct 16, 2018
Merged

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Oct 5, 2018

What was wrong?

Vyper Registry contract - following ERC-1319
Including the registry.v.py contract in /contracts/ for now, with plans to yank it out or move it to /tests/ later

@fubuloubu @jacqueswww @davesque Would love some feedback on the contract / testing. It's still a WIP as there is plenty of room for improvements

some concerns:

  • Right now max # of packages/releases allowed per registry = 128 (number is irrelevant, but i'm still looking for a way to improve this - variable length arrays are not possible, correct?)
  • docs nonexistent, but coming soon

CI won't pass until ethereum/pytest-ethereum#11 is merged and released

Cute Animal Picture

image

tests/core/test_registry_contract.py Outdated Show resolved Hide resolved


def test_registry_get_package_name_raises_exception_if_package_doesnt_exist(registry):
with pytest.raises(TransactionFailed):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is one of the patterns that we likely want to make easier with pytest ethereum (as suggested by @fubuloubu ), could just be a very thin wrapper around pytest.raises to start with but something like:

import pytest_ethereum as pte  # we should establish a convention, don't know if I like this one

with pte.tx_fail():
    ...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransactionFailed could also be a "default" for such a wrapper (that can be overridden)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fubuloubu like this idea - but i'm also partial to explicitly writing the exception name. i.e. tx_fail. in which case if you want to override it, you could just use pytest.raises(xxx) - or am i missing something? can you give an sample api of what you were thinking?

tx_hash = registry.functions.release(b"package", b"1.0.0", b"google.com").transact()
receipt = w3.eth.waitForTransactionReceipt(tx_hash)
logs = registry.events.Release().processReceipt(receipt)
assert logs[0]["args"]["_package"][:7] == b"package"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, another pattern we want to make easier (as suggested by @fubuloubu )

Here are some ideas for starting points.

# test specific log
log = ...
assert Log(registry.events.Release, "package", "1.0.0", "google.com").match(log)

# or maybe test it is just present in a receipt
receipt = ...
assert Log(registry.events.Release, "package", "1.0.0", "google.com").is_present(receipt)

# or test that there is no log entry
assert Log(registry.events.Release, "package", "1.0.0", "google.com").not_present(receipt)

The Log constructor would need to do some magic introspecting the ContractEvent object returned by registry.events.Receipt but I think all the necessary information is there.

tests/core/test_registry_contract.py Outdated Show resolved Hide resolved

package_count: public(int128)
release_count: public(int128)
package_ids: bytes32[128]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very curious what the vyper pattern would be to allow arbitrary numbers of releases. Can we just use an arbitrarily large number here instead of 128?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to work on a stack widget, which could solve this. Constant time insertions and accesses. Still not sure if it should be a full double-linked list ($$$ gas) or a very simple push/pop data structure with no ability to pop anything other than the end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njgheorghita (maybe @jacqueswww or @fubuloubu can confirm)

What about this API which is in the vyper by example section:

https://vyper.readthedocs.io/en/latest/vyper-by-example.html#crowdfund

package_ids: {id: bytes32}[int128]

Wouldn't that allow you to have an effectively infinite psuedo-array of them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I assume it should be possible to do a mapping to a non-object type such as int128 => bytes32 but it wasn't clear what that syntax would be...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that would be the most efficient possibility. You would have to manage a separate "length" parameter to keep track of what the last entry you set was though.

A stack type widget could manage that for you under the hood. Same with a queue type.

@njgheorghita
Copy link
Collaborator Author

@fubuloubu @pipermerriam made the suggested changes, ci won't pass until ethereum/pytest-ethereum#11 is merged/released

mostly added a mechanism to allow getting all releases for a package, it seems to work alright, but not sure if there's a better way to achieve that goal? open to any/all suggestions for improvement

@pipermerriam
Copy link
Member

So this is now compliant with ERC1319?

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a rubber stamp. Can you get an issue open to track that this contract will need to be audited in some fashion prior to releasing a beta of twig?

offset: int128, length: int128
) -> (bytes32, bytes32, bytes32, bytes32, bytes32):
# todo update to uint256?
assert length == 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is how you make this compliant. I'd be curious if there's a way to loosen the restriction such that length <= 5. Doesn't have to block you moving this forward.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can return fixed arrays I think. You can definitely return bytes[160], if not bytes32[5]

@njgheorghita njgheorghita force-pushed the init-dev branch 4 times, most recently from 6ca5bf6 to a1c8d50 Compare October 11, 2018 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants